Jsx ast #7286

nojaf · 2025-02-08T18:46:37Z

This is a part of #7283.
I'm introducing Pexp_jsx_fragment to represent fragment syntax <></>.

I found it insightful to just try it out and see what code changes are necessary.

In short a fragment is now parsed as:

        expression 
          attribute  "JSX"
            []
          Pexp_construct "::" 
          Some
            expression 
              Pexp_tuple
              [
                expression 
                  attribute  "JSX"
                    []
                  Pexp_apply
                  expression 
                    Pexp_ident "SectionHeader.createElement" 
                  [
                    <arg>
                    Labelled "children"
                      expression 
                        Pexp_construct "::" 
                        Some
                          expression 
                            Pexp_tuple
                            [
                              expression 
                                attribute  "res.braces"
                                  []
                                Pexp_apply
                                expression 
                                  Pexp_ident "React.string" 
                                [
                                  <arg>
                                  Nolabel
                                    expression 
                                      Pexp_constant PConst_string ("abc",Some "*j")
                                ]
                              expression 
                                Pexp_construct "[]" 
                                None
                            ]
                    <arg>
                    Nolabel
                      expression 
                        Pexp_construct "()" 
                        None
                  ]
                expression 
                  Pexp_construct "[]" 
                  None
              ]

after this change it becomes:

        expression 
          Pexp_jsx_fragment          [
            expression 
              attribute  "JSX"
                []
              Pexp_apply
              expression 
                Pexp_ident "SectionHeader.createElement" 
              [
                <arg>
                Labelled "children"
                  expression 
                    Pexp_construct "::" 
                    Some
                      expression 
                        Pexp_tuple
                        [
                          expression 
                            attribute  "res.braces"
                              []
                            Pexp_apply
                            expression 
                              Pexp_ident "React.string" 
                            [
                              <arg>
                              Nolabel
                                expression 
                                  Pexp_constant PConst_string ("abc",Some "*j")
                            ]
                          expression 
                            Pexp_construct "[]" 
                            None
                        ]
                <arg>
                Nolabel
                  expression 
                    Pexp_construct "()" 
                    None
              ]
          ]

I'll add some comment to relevant changes.

nojaf · 2025-02-08T18:48:10Z

compiler/syntax/src/res_core.ml


 (*
 * jsx-fragment ::=
 *  | <> </>
 *  | <> jsx-children </>
 *)
-and parse_jsx_fragment p =
+and parse_jsx_fragment start_pos p =


I prefer ranges to be accurate. The location should start at the opening < token.

nojaf · 2025-02-08T18:50:55Z

compiler/syntax/src/res_printer.ml

  Doc.group
    (Doc.concat
       [
+         line_sep;


@shulhi this part isn't 100% correct.
I'm not familiar enough with this part of the codebase.

let z1 = <> <SectionHeader> {React.string("abc")} </SectionHeader> </>

will be formatted to

let z1 = <> <SectionHeader> {React.string("abc")} </SectionHeader> </>

so I'm missing some indent here. Not sure how that part works.
Would love to hear your thoughts on this.

@nojaf I checked, the indentation is handled here,

rescript/compiler/syntax/src/res_printer.ml

Lines 2085 to 2103 in 9da9b19

let should_indent =

match opt_braces with

| Some _ -> false

| _ -> (

ParsetreeViewer.is_binary_expression expr

||

match vb.pvb_expr with

| {

pexp_attributes = [({Location.txt = "res.ternary"}, _)];

pexp_desc = Pexp_ifthenelse (if_expr, _, _);

} ->

ParsetreeViewer.is_binary_expression if_expr

|| ParsetreeViewer.has_attributes if_expr.pexp_attributes

| {pexp_desc = Pexp_newtype _} -> false

| {pexp_attributes = [({Location.txt = "res.taggedTemplate"}, _)]} ->

false

| e ->

ParsetreeViewer.has_attributes e.pexp_attributes

|| ParsetreeViewer.is_array_access e)

You might need to handle the case for Pexp_jsx_fragment here.

Thanks, the old code will instead have matched ParsetreeViewer.has_attributes e.pexp_attributes. Indentation now works as before.

nojaf · 2025-02-08T18:51:45Z

tests/analysis_tests/tests/src/expected/Completion.res.txt

@@ -1000,7 +1000,7 @@ Path Objects.Rec.

 Complete src/Completion.res 120:7
 posCursor:[120:7] posNoWhite:[120:6] Found expr:[119:11->123:1]
-posCursor:[120:7] posNoWhite:[120:6] Found expr:[120:5->122:5]
+posCursor:[120:7] posNoWhite:[120:6] Found expr:[120:5->123:0]


Changes like this are to be expected as the range of a fragment spans from <> till end </>.

nojaf · 2025-02-08T18:53:35Z

tests/analysis_tests/tests/src/expected/Fragment.res.txt

 posCursor:[9:56] posNoWhite:[9:55] Found expr:[9:13->9:66]
 JSX <SectionHeader:[9:13->9:26] > _children:9:26
-posCursor:[9:56] posNoWhite:[9:55] Found expr:__ghost__[9:10->9:67]


This ghost expression is a part of the AstHelper.make_list_expression result.
It is no longer present in the new AST, but it also didn't serve any purpose.
I believe it is okay that this test is slightly different.
In the end the result didn't change.

nojaf · 2025-02-08T18:56:56Z

compiler/ml/parsetree.ml

+  (* [%id] *)
+  (* . *)
+  (* represents <> foo </> , the entire range is stored in the expression , we keep track of >, children and </ *)
+  | Pexp_jsx_fragment of


The reason the store the > and </ token is this edge case:
https://rescript-lang.org/try?version=v12.0.0-alpha.8&module=esmodule&code=DYUwLgBA+hC8ECgCQAeCB6AVBAzmATgIYB2A5iBJuhAHzJIDeASiIQMZgB0e+AlmQAoARAFsQACyEBKAL7IU1LBEIiIASQh9S4yFVpA

If we ever want to restore comments I suppose we need the proper anchors.

compiler/syntax/src/res_core.ml

compiler/syntax/src/res_comments_table.ml

tests/syntax_tests/data/printer/expr/expected/braced.res.txt

tests/syntax_tests/data/printer/comments/expected/jsx.res.txt

Map child expressions Initial mapping of Pexp_jsx_fragment to 0 Correct location in mapping Update analysis for jsx_fragment Remove unused code Print something for ml print Commit invalid test results for reference Try improve printing Correct fragment range, try and print comments Indent jsx Process comments from children inside fragment Attach comments to fragment tags Fix comment Improve comment formatting Print single element on same line Update comment WIP: Debug More debugging Works Fix some jsx printing Fix the test Clean up Update tests with location changes

nojaf · 2025-02-21T09:17:06Z

Thank you @shulhi for fixing all the remaining formatter problem of the fragment node!
The fragment node is now in a good state.
@cristianoc could you review this PR before I continue to introduce a new node for jsx_element.

cristianoc

Left some comments.
Just checking: is the conversion complete in that the representation of fragments is completely moved over to the new representation?

analysis/src/CompletionFrontEnd.ml

cristianoc · 2025-02-21T17:33:56Z

compiler/ml/ast_mapper_to0.ml

@@ -407,6 +407,18 @@ module E = struct
    | Pexp_open (ovf, lid, e) ->
      open_ ~loc ~attrs ovf (map_loc sub lid) (sub.expr sub e)
    | Pexp_extension x -> extension ~loc ~attrs (sub.extension sub x)
+    | Pexp_jsx_fragment (o, xs, c) ->


There should be a corresponding change in ast_mapper_from0 that inverts the conversion.
Also, some extension of the test cases in tests/tools_tests/ppx/TestPpx.res to check that back and forth conversion works fine.

Alright, I think the new tests from #7318 prove this now.

nojaf · 2025-02-22T09:44:47Z

Left some comments. Just checking: is the conversion complete in that the representation of fragments is completely moved over to the new representation?

Yes, that is the idea. The parser only produces jsx_fragment and not the old representation.
Besides the ast_mapper_from0 I believe everything is dealt with.

shulhi · 2025-02-27T22:02:15Z

tests/tools_tests/ppx/TestPpx.res

@@ -61,3 +61,7 @@ let eq2 = 3 === 3

 let test = async () => 12
 let f = async () => (await test()) + 1
+
+module Fragments = {
+  let f1 = <> </>


This should be <></>, no? (without the space).

Sounds reasonable, although I do believe this is the current behavior. (Playground)

nojaf · 2025-03-03T09:17:00Z

@cristianoc, anything else you can think of for jsx_fragment?
If not, we can start looking at other jsx syntax.

cristianoc · 2025-03-03T09:40:14Z

@cristianoc, anything else you can think of for jsx_fragment? If not, we can start looking at other jsx syntax.

This could be ready to go. Is the absence of changes in ast-mapping what tells us that the back and forth conversion is just as before adding the fragment node?

cristianoc · 2025-03-03T09:41:41Z

Would you add a changelog too? Assuming this change will go ahead, and the rest of the JSX investigated in a separate PR.

nojaf · 2025-03-03T09:57:46Z

This could be ready to go. Is the absence of changes in ast-mapping what tells us that the back and forth conversion is just as before adding the fragment node?

Yes, I understand. Today, I made some necessary changes to ensure the tests remained consistent.

Would you add a changelog too? Assuming this change will go ahead, and the rest of the JSX investigated in a separate PR.

I would recommend investigating this in the same PR. There isn't much benefit to having just the fragments part. To avoid the risk of not completing a second PR, I would suggest keeping it as one.

That being said, I would like to achieve a sense of completion for the fragments before taking on more changes.

cristianoc · 2025-03-03T13:40:54Z

Sure continuing jsx sounds good.

Handle comments and printing for jsx_unary_tag

Handle comments and printing (part 2)

nojaf · 2025-03-26T16:07:41Z

Hi @shulhi,

Did this test ever passed for you?

I'm wondering if I regressed things or not with the comments.

Run opam exec -- make test-syntax-roundtrip to see.

shulhi · 2025-03-27T01:32:42Z

Hi @shulhi,

Did this test ever passed for you?

I'm wondering if I regressed things or not with the comments.

Run opam exec -- make test-syntax-roundtrip to see.

Do you mean on this branch or master? I think comments handling need a bit more work still which I'm currently working on. I'll update you later regarding these specific tests.

nojaf · 2025-03-27T07:32:45Z

Do you mean on this branch or master?

I noticed that some roundtrip tests are still losing comments on this branch. I’m wondering if this issue arose after your changes or if it still needs some work.

shulhi · 2025-03-27T12:49:11Z

Do you mean on this branch or master?

I noticed that some roundtrip tests are still losing comments on this branch. I’m wondering if this issue arose after your changes or if it still needs some work.

Definitely needs some more work. I'm working on the JSX container tag printing/handling comments. Then, need to fix the broken tests. They are expected to fail at this point until I get everything implemented.

nojaf · 2025-03-28T13:43:38Z

compiler/syntax/src/res_comments_table.ml

+               Some (ExprArgument {expr; loc})
+             | Parsetree.JSXPropValue ({loc}, _, expr) ->
+               let loc = {loc with loc_end = expr.pexp_loc.loc_end} in
+               Some (ExprArgument {expr; loc})


@shulhi I struggle a bit to grasp this part.
What should happen in the follow scenario:

<Foo bar="hey" // comment ></Foo>

Is // comment linked to "hey" or the entire bar="hey" expression?
The loc created here is combining the prop name and value but is further processed with only the value expression?
This feel off, is this by design? If so, I assume you then compensate for this in res_printer?
Would it not make more sense to either introduce a new node type or create a helper function and process the three parts (entire prop, prop name & prop value)?

nojaf commented Feb 8, 2025

View reviewed changes

nojaf commented Feb 10, 2025

View reviewed changes

compiler/syntax/src/res_core.ml Show resolved Hide resolved

nojaf commented Feb 13, 2025

View reviewed changes

compiler/syntax/src/res_comments_table.ml Outdated Show resolved Hide resolved

nojaf commented Feb 14, 2025

View reviewed changes

tests/syntax_tests/data/printer/expr/expected/braced.res.txt Show resolved Hide resolved

nojaf commented Feb 14, 2025

View reviewed changes

tests/syntax_tests/data/printer/comments/expected/jsx.res.txt Outdated Show resolved Hide resolved

nojaf force-pushed the jsx-ast branch from 9451cd2 to db7eee6 Compare February 21, 2025 09:15

cristianoc reviewed Feb 21, 2025

View reviewed changes

nojaf added 2 commits February 24, 2025 13:38

Initial mapping from0

28aa287

Format code

b57f2df

shulhi reviewed Feb 27, 2025

View reviewed changes

nojaf added 4 commits March 3, 2025 07:55

Merge branch 'master' into jsx-ast

32161a3

Fix jsx fragment mapping

d714829

Remove fragment

e27b339

Update test output

92657ee

nojaf added 8 commits March 6, 2025 11:55

Introducing Pexp_jsx_unary_element & Pexp_jsx_container_element

441de6e

Refactor fragment transformation.

bf1b411

Initial transform of Pexp_jsx_unary_element in automatic mode

5fe5f12

Make props for unary element in automatic mode.

9251609

Initial custom component unary tag

1aaa686

lowercase container element with children.

b374212

Uppercase container elements

c91aeda

Streamline automatic element calls

85ccab4

shulhi and others added 19 commits March 22, 2025 09:34

Fix closing tag indentation

aaef8ac

Fix closing tag indentation for cases with break vs inline

b863888

Handle some edge cases

7f8bf01

Refactor

4e0951c

Merge pull request #3 from shulhi/shulhi-jsx-ast

7843795

Handle comments and printing for jsx_unary_tag

WIP: Handle punning

9e68c1a

Fix optional printing with braces

53616a7

WIP: Handle comments attachment in a different way

25e20b8

Handle empty props

ad72bf2

Handle props spread

7ba4996

Fix optional with punning

f09cd70

Merge pull request #4 from shulhi/shulhi-jsx-ast-2

aa1d978

Handle comments and printing (part 2)

Indent props if they don't fit on one line.

0ee7601

Fix indentation of children when props are multiline.

bd45146

Refactor to Pexp_jsx_element

2d50ff8

Merge branch 'master' into jsx-ast

fcabcff

Don't always append make when uppercase component

ddcdaa2

Use Doc.line for child spreading

52ecf03

Exotic prop name

cd9059f

Don't create record if only spreading prop, use that expression instead.

2e9fb66

nojaf added 3 commits March 27, 2025 08:41

Key prop can be optional

37755ae

Keep comment after opening greater than

3a434c9

Print prop value with comments

6cf1e71

nojaf added 2 commits March 27, 2025 16:18

Update generic jsx completion tests

ae9b428

Revert isJsxComponent

f3b96df

nojaf commented Mar 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jsx ast #7286

Jsx ast #7286

nojaf commented Feb 8, 2025

nojaf Feb 8, 2025

nojaf Feb 8, 2025

shulhi Feb 11, 2025 •

edited

Loading

nojaf Feb 11, 2025

nojaf Feb 8, 2025

nojaf Feb 8, 2025

nojaf Feb 8, 2025

nojaf commented Feb 21, 2025

cristianoc left a comment

cristianoc Feb 21, 2025

nojaf Mar 3, 2025

nojaf commented Feb 22, 2025

shulhi Feb 27, 2025

nojaf Feb 28, 2025

nojaf commented Mar 3, 2025

cristianoc commented Mar 3, 2025

cristianoc commented Mar 3, 2025

nojaf commented Mar 3, 2025

cristianoc commented Mar 3, 2025

nojaf commented Mar 26, 2025

shulhi commented Mar 27, 2025

nojaf commented Mar 27, 2025

shulhi commented Mar 27, 2025

nojaf Mar 28, 2025

	let should_indent =
	match opt_braces with
	\| Some _ -> false
	\| _ -> (
	ParsetreeViewer.is_binary_expression expr
	\|\|
	match vb.pvb_expr with
	\| {
	pexp_attributes = [({Location.txt = "res.ternary"}, _)];
	pexp_desc = Pexp_ifthenelse (if_expr, _, _);
	} ->
	ParsetreeViewer.is_binary_expression if_expr
	\|\| ParsetreeViewer.has_attributes if_expr.pexp_attributes
	\| {pexp_desc = Pexp_newtype _} -> false
	\| {pexp_attributes = [({Location.txt = "res.taggedTemplate"}, _)]} ->
	false
	\| e ->
	ParsetreeViewer.has_attributes e.pexp_attributes
	\|\| ParsetreeViewer.is_array_access e)

Jsx ast #7286

Are you sure you want to change the base?

Jsx ast #7286

Conversation

nojaf commented Feb 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shulhi Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Feb 21, 2025

cristianoc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Feb 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nojaf commented Mar 3, 2025

cristianoc commented Mar 3, 2025

cristianoc commented Mar 3, 2025

nojaf commented Mar 3, 2025

cristianoc commented Mar 3, 2025

nojaf commented Mar 26, 2025

shulhi commented Mar 27, 2025

nojaf commented Mar 27, 2025

shulhi commented Mar 27, 2025

Choose a reason for hiding this comment

shulhi Feb 11, 2025 •

edited

Loading